Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing Issues with CrossPlane Integrations. #20

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

elamaran11
Copy link
Contributor

@elamaran11 elamaran11 commented Jul 30, 2024

This PR Does the following :

  1. Fixing Issues with CrossPlane Integrations.

Copy link
Contributor

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The change to the ArgoCD spec looks good. I am not sure if I want to add RDS compositions though. Looks like this was taken from the crossplane on eks repo. I would rather not duplicate things here.

Also I don't think I want to add RDS without proper support for it in Backstage.

@nimakaviani
Copy link
Contributor

thanks @elamaran11 for this.

I agree with Manabu on the compositions. Looks like we would want to take the crossplane compositions under https://github.com/cnoe-io/backstage-crossplane-integrations and couple them with the corresponding Backstage templates, similar to what we have done for Terraform. wdyt?

@elamaran11
Copy link
Contributor Author

elamaran11 commented Jul 31, 2024

@nimakaviani @nabuskey As part of Manabu's concern i already have this PR in backstage-crossplane-integrations repo to add Backstage support for RDS. His second concern was duplicating from crossplane-on-eks but we have to duplicate it somewhere for seamsless delivery of these patterns. I have no issues in removing RDS compositions and moving it to https://github.com/cnoe-io/backstage-crossplane-integrations but as part of consistency then the S3 compositions should also be removed from this repo and the crossplane-compositons.yaml will make no sense should also be removed from this repo because currently it points to composition folder in stacks (in this repo). So i recommend adding RDS compositions to this repo or If you insists to move compositions to the other repo, then you need another Argo Application for compositions in backstage-crossplane-integrations repo. Basically we have to work on rearchitecture to

  1. First remove crossplane-compositions.yaml and compositions folder from this repo to backstage-crossplane-integrations repo
  2. Create a new Argo Application in backstage-crossplane-integrations repo
  3. Move the S3 compositions and add RDS and all new compositions to backstage-crossplane-integrations repo and maintain it there.

Overall the end result is same but we arent really achieving a lot with this approach vs doing a lot of moving of stuff here and there. Please let me know WY both think, i can find time next week to work on this.

@nimakaviani
Copy link
Contributor

Thanks @elamaran11 ... I agree with your point about also moving the s3 compositions and the related backstage templates over to the backstage-crossplane-integrations repo. We initially didn't do it because that repo did not exist and also we dont have proper instructions on the website on how to get the stuff to work together.

Lets work on consolidating everything under backstage-crossplane-integrations

@elamaran11 elamaran11 changed the title Fixing Issues with CrossPlane Integrations and Adding RDS Compositions Fixing Issues with CrossPlane Integrations. Aug 1, 2024
@elamaran11
Copy link
Contributor Author

@nimakaviani Thankyou for agreeing to my approach. I will work on another PR to stacks repo and crossplane integrations repo to consolidate crossplane compositions. @nimakaviani @nabuskey Please merge this PR which has bug fix to fix a show stopper with crossplane integrations because crossplane currently fails without this fix.

Copy link
Contributor

@nabuskey nabuskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nabuskey nabuskey merged commit ba99dc9 into cnoe-io:main Aug 1, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants